Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

python3-opencv should be a more general dependency #439

Merged
merged 1 commit into from
Jul 15, 2022
Merged

python3-opencv should be a more general dependency #439

merged 1 commit into from
Jul 15, 2022

Conversation

ijnek
Copy link
Member

@ijnek ijnek commented Jun 18, 2022 •
edited
Loading

cv2 is being used as a dependency at run time:

vision_opencv/cv_bridge/python/cv_bridge/core.py

Line 71 in 0b69de3

import cv2

and hence should be listed not only as a test dependency, but as a general dependency, like python3-numpy is a couple of lines above the change in this PR.

@ijnek ijnek mentioned this pull request Jun 18, 2022
Merged
@ijnek
Copy link
Member Author

ijnek commented Jun 18, 2022

I believe the CI failure is due to a separate reason. The exact same error shows up when testing on the ros2 branch.

@ijnek
Copy link
Member Author

ijnek commented Jun 29, 2022

Friendly ping @vrabaud

@ijnek ijnek mentioned this pull request Jun 30, 2022
Closed
@ijnek
Copy link
Member Author

ijnek commented Jul 5, 2022

Or @gaoethan ?

@ijnek
Copy link
Member Author

ijnek commented Jul 13, 2022 •
edited
Loading

Perhaps @audrow, could you have a look at these changes please?

audrow
audrow reviewed Jul 13, 2022
View reviewed changes
Copy link

@audrow audrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me, but I'd like the PR job to pass before we merge (as was done here).

It's suspicious that the failures are coming on the PR jobs on Jammy (Rolling and Humble), but I don't see why this change would result in those failures.

@audrow
Copy link

audrow commented Jul 13, 2022

@ros-pull-request-builder retest this please

@ijnek
Copy link
Member Author

ijnek commented Jul 14, 2022 •
edited
Loading

CI Failed again wtih error:

03:17:09 2: ==================================== ERRORS ====================================
03:17:09 2: ________________ ERROR collecting cv_bridge/test/enumerants.py _________________
03:17:09 2: ImportError while importing test module '/tmp/ws/src/vision_opencv/cv_bridge/test/enumerants.py'.
03:17:09 2: Hint: make sure your test modules/packages have valid Python names.
03:17:09 2: Traceback:
03:17:09 2: /usr/lib/python3.10/importlib/__init__.py:126: in import_module
03:17:09 2:     return _bootstrap._gcd_import(name[level:], package, level)
03:17:09 2: /tmp/ws/src/vision_opencv/cv_bridge/test/enumerants.py:4: in <module>
03:17:09 2:     from cv_bridge import CvBridge, CvBridgeError, getCvType
03:17:09 2: E   ModuleNotFoundError: No module named 'cv_bridge'
03:17:09 2: -- generated xml file: /tmp/ws/test_results/cv_bridge/enumerants.py.xunit.xml --
03:17:09 2: =========================== short test summary info ============================
03:17:09 2: ERROR ../../../src/vision_opencv/cv_bridge/test/enumerants.py
03:17:09 2: !!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
03:17:09 2: =============================== 1 error in 0.35s ===============================
03:17:09 2: -- run_test.py: return code 2
03:17:09 2: -- run_test.py: verify result file '/tmp/ws/test_results/cv_bridge/enumerants.py.xunit.xml'
03:17:09 2/4 Test #2: enumerants.py ....................***Failed    0.73 sec

which I believe is what #444 is trying to solve (hint - that PR is the only one passing CI). I haven't looked into the PR properly, but I'm not quite sure if that's the way it should be solved...

@audrow audrow self-assigned this Jul 14, 2022
@audrow
Copy link

audrow commented Jul 14, 2022

which I believe is what #444 is trying to solve (hint - that PR is the only one passing CI). I haven't looked into the PR properly, but I'm not quite sure if that's the way it should be solved...

I think you're right. Pinging the maintainers: @vrabaud, it looks like the PR jobs are failing from an unrelated problem. Should we merge this?

@clalancette
Copy link
Contributor

I agree that this is a general dependency. I know that CI is failing, but hopefully #444 will fix that. That said, this looks valid so I'm going to go ahead and merge this one.

@clalancette clalancette merged commit 2483884 into ros-perception:ros2 Jul 15, 2022
@ijnek ijnek deleted the ijnek-cv2-dependency branch July 15, 2022 23:51
@ijnek
Copy link
Member Author

ijnek commented Jul 16, 2022

Thanks @audrow and @clalancette

@ijnek ijnek mentioned this pull request Sep 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@audrow audrow audrow left review comments

Assignees

@audrow audrow

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ijnek @audrow @clalancette